Fix generics and some formatting - #59
Conversation
| public AbstractJiraServerInfo parse( Object object ) | ||
| { | ||
| Map<String, Object> map = (Map<String, Object>) object; | ||
| Map<?, ?> map = (Map<?, ?>) object; |
There was a problem hiding this comment.
I still think it would be better to use @SuppressWarnings( "unchecked" ) here rather than the wildcard parameterised type ; this would then match your changes in e.g. Registry.java and ParseUtils.
There was a problem hiding this comment.
I think I have a simple way to explain it. The problem is where the cast is (first line vs. second line):
Wrong way:
Map<String, Integer> map = (Map<String, Integer>) obj; // wrong: unchecked cast
Integer i = map.get( "x" ); // compiler can't verify this, can throw ClassCastExceptionRight way:
Map<?, ?> map = (Map<?, ?>) obj;
Integer i = (Integer) map.get( "x" ) // checked cast hereWhen I use @SuppressWarnings("unchecked") in Registry and ParseUtils, that was where it was safe.
In other words, cast the get not the Map.
There was a problem hiding this comment.
Full disclosure - Bob helped here!
You're right in that the general principal is valid ( i.e. Casting individual values is safer than casting the container ). However every value is already cast individually below. So changing the map declaration from Map<String, Object> to Map adds nothing to safety.
What the change does introduce is an asymmetry: render() in the same class still declares Map<String, Object> on line 49, since it constructs the map and knows its types. Having parse() use Map for the same logical structure makes the two halves of the same converter inconsistent.
A narrowly scoped @SuppressWarnings("unchecked") on the single cast line — as you did in ParseUtils — would acknowledge the one genuinely unchecked operation, preserve the Map<String, Object> type for the rest of the method, and keep parse() and render() symmetric. That is why I still think it is the better approach for this particular class.
There was a problem hiding this comment.
True, there is an asymmetry there, but it's intended: render() calls put(String, Object), so it needs to know the type, while parse() uses only plain Object. The symmetry before came from parse() asserting a type that the compiler can't verify, which is exactly the thing to eliminate.
The cases in ParseUtils and Registry are cases where wildcards can't be used. It's not an open choice. The warnings could not be eliminated there, which is why I kept @SuppressWarnings there.
After this PR, there will be no instances of (Map<String, Object>) left in the codebase. Right now, this line, JiraServerInfoConverter.java:32, seems to be the only one in dispute. Combined with #48, this will eliminate all cases of (Map<String, Object>) as well as raw Map and List that were in the codebase. One aside is that the warnings in the generated code were also being passed onto all consumers like kojiji.
50702e3 to
ee580c0
Compare
ee580c0 to
7f3437a
Compare
Mend Scan ResultsStatus: ✅ No findings detected SCA scan outputSAST scan output |
No description provided.